Skip to content

fix: don't re-observe a detached element after destroy(), and clean up a removed child on disable() - #247

Open
AndreasOhlsson wants to merge 2 commits into
formkit:masterfrom
AndreasOhlsson:fix/observe-detached-element
Open

AndreasOhlsson wants to merge 2 commits into
formkit:masterfrom
AndreasOhlsson:fix/observe-detached-element

Conversation

@AndreasOhlsson

@AndreasOhlsson AndreasOhlsson commented Oct 8, 2026 •

Copy link
Copy Markdown

Fixes #246, and a second 0.10 cleanup bug in disable() (below).

What happens

A position update queued with lowPriority() (requestIdleCallback) before destroy() can still run after it, for example when the component unmounts during a busy route change. updatePos(el) then calls observePosition(el), which creates a new IntersectionObserver rooted at document.documentElement on the now-detached element. Nothing disconnects that observer, and it keeps the detached subtree alive for the rest of the session.

The fix

observePosition() returns early for an element that is no longer in the document, after disconnecting any old observer. Every path that (re)creates an observer goes through this function, so one guard covers all of them:

  • idle-callback polls
  • updateAllPos
  • observer callbacks
  • animation finished handlers

Observing a disconnected element measures nothing, so no animation behaviour changes for connected elements.

Evidence

  • Repro: in Chromium, a parent with 10 children; unmount it during a ~600 ms busy main-thread window; force GC.
    • 0.10.0: the detached subtree survived in 11 of 12 trials. Every leaked observer was created 200–600 ms after an idle callback that was queued before destroy() and ran after it.
    • With this guard: 0 of 12 leaked, even though idle callbacks still ran after destroy().
  • In a production React app (useAutoAnimate on a timeline that remounts on navigation), over 60 navigations:
    • 0.10.0: about +120 DOM nodes per navigation.
    • With the guard (applied as a patch): DOM nodes and event listeners stay flat.

Relation to #242

#242 (which fixes #180) tracks the poll start timers and stops polling in cleanup. That closes the timer-side leaks, but an idle callback that is already queued still reaches observePosition() after destroy(). The two changes complement each other and don't conflict.

I didn't add a test. The race needs a busy main thread around unmount, which the existing coarse memory.spec.ts doesn't cover. Happy to add a targeted e2e test modelled on it if you'd like one.

Second fix: disable() during a removal leaves the removed child behind

What happens

Suppose disable() is called while a removed child is still playing its exit animation:

  • That child stays in the DOM permanently, absolutely positioned at its old place.
  • The parent stops animating. getElements returns early while any child still carries the delete marker, so later adds and removes no longer animate until the parent unmounts.

Why it happens

In 0.10, disable() cancels every running animation on the parent's children. A cancelled animation never fires finish, and the finish listener is what calls cleanUp for a removed child. In 0.8.2, disable() only removed the parent from the enabled set.

Steps

  1. const controller = autoAnimate(list), with the default 250 ms duration.
  2. Remove a child.
  3. Call controller.disable() within 250 ms.
  4. The removed child stays, and the list no longer animates.

The fix

disable() now finishes the animation of a child marked for deletion, which runs its finish handler and so cleanUp. Every other child is still cancelled, so disable() still stops in-flight animations immediately.

Where we hit it: a list that disables animations while it preserves scroll position. Un-including one row removes it, and toggling another row within 250 ms disables the list mid-exit. We run this change as a local patch, and the ghost row is gone.

I can split this into its own PR if you'd prefer one fix per PR.

Diagnosed with heap snapshots and a Playwright repro.

@vercel

vercel Bot commented Oct 8, 2026

Copy link
Copy Markdown

@AndreasOhlsson is attempting to deploy a commit to the Formkit Team on Vercel.

A member of the Team first needs to authorize it.

A position update queued with lowPriority() before destroy() can run after
it. updatePos() then calls observePosition() on the detached element and
creates a new IntersectionObserver rooted at document.documentElement, which
keeps the detached subtree alive for the rest of the session.

Fixes formkit#246
@AndreasOhlsson
AndreasOhlsson force-pushed the fix/observe-detached-element branch from 6d40a49 to c029f01 Compare October 8, 2026 10:35
disable() cancels every running animation on the parent's children. A
cancelled animation never fires "finish", so a child removed just before
disable() never reaches cleanUp: it stays in the DOM, absolutely
positioned at its old place, and keeps its delete marker, so getElements
bails and the parent stops animating until it unmounts.

Finish the animation of a child marked for deletion instead, which runs
its finish handler and so cleanUp. Every other child is still cancelled.
@AndreasOhlsson AndreasOhlsson changed the title fix: don't re-observe a detached element after destroy() fix: don't re-observe a detached element after destroy(), and clean up a removed child on disable() Oct 13, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0.10.0: destroy() can leave an IntersectionObserver on a detached element (idle callback re-arms observePosition) Memory Leak

1 participant